Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Filter & from tag name #959

Merged
merged 2 commits into from
Dec 7, 2024
Merged

fix: Filter & from tag name #959

merged 2 commits into from
Dec 7, 2024

Conversation

angrybayblade
Copy link
Collaborator

@angrybayblade angrybayblade commented Dec 6, 2024

Important

Update get_enum_key in enums.py to replace & with _ in tag names and modify an error message in test_toolset.py.

  • Behavior:
    • Update get_enum_key in enums.py to replace & with _ in tag names.
  • Tests:
    • Update error message in test_uninitialize_app() in test_toolset.py to use attio instead of linear.

This description was created by Ellipsis for e3f7265. It will automatically update as commits are pushed.

Copy link

vercel bot commented Dec 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
composio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 7, 2024 9:27am

@angrybayblade angrybayblade changed the title Filter & from tag name fix: Filter & from tag name Dec 6, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to a880fdf in 9 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/composio/utils/enums.py:5
  • Draft comment:
    Consider using a set for characters_to_replace for faster membership testing, although the current list is small and performance impact is minimal.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The change is straightforward and correct, but there's a potential performance improvement.

Workflow ID: wflow_kYixT7rXxwqcvItb


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@@ -2,7 +2,7 @@


def get_enum_key(name: str) -> str:
characters_to_replace = [" ", "-", "/", "(", ")", "\\", ":", '"', "'", "."]
characters_to_replace = [" ", "-", "/", "(", ")", "\\", ":", '"', "'", ".", "&"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider defining this list as a module-level constant to improve maintainability and reusability. Example:

SPECIAL_CHARACTERS = [" ", "-", "/", "(", ")", "\\", ":", '"', "'", ".", "&"]

def get_enum_key(name: str) -> str:
    for char in SPECIAL_CHARACTERS:
        name = name.replace(char, "_")
    return name.upper()

@@ -2,7 +2,7 @@


def get_enum_key(name: str) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding docstring to improve code documentation:

def get_enum_key(name: str) -> str:
    """Convert a string into a valid enum key by replacing special characters with underscores.
    
    Args:
        name (str): The input string to be converted
        
    Returns:
        str: The converted string in uppercase with special characters replaced by underscores
        
    Example:
        >>> get_enum_key("Hello & World")
        'HELLO_WORLD'
    """

@shreysingla11
Copy link
Collaborator

Code Review Summary

The change looks good and addresses the issue of handling & characters in tag names. The implementation is consistent with the existing code pattern and maintains backward compatibility.

Strengths:

✅ Simple and focused fix
✅ Consistent with existing code style
✅ Addresses a specific issue with tag name handling
✅ No breaking changes

Suggestions for improvement:

  1. Consider moving the characters list to a module-level constant
  2. Add docstring to improve code documentation
  3. Consider adding specific unit tests for & character handling

Overall, the PR is ready to merge with the suggested improvements being optional for future enhancements.

Copy link

github-actions bot commented Dec 6, 2024

This comment was generated by github-actions[bot]!

JS SDK Coverage Report

📊 Coverage report for JS SDK can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/coverage-12211774412/coverage/index.html

📁 Test report folder can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/html-report-12211774412/html-report/report.html

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on e3f7265 in 7 seconds

More details
  • Looked at 21 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/tests/test_tools/test_toolset.py:52
  • Draft comment:
    The test case was updated to reflect changes in the application name from linear to attio. Ensure that this change is consistent with the rest of the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test case test_uninitialize_app was updated to reflect changes in the application name from linear to attio. This change aligns with the PR description, which mentions updating tag names. The test case now correctly tests for the attio app, ensuring that the error message and action schema are consistent with the new application context.

Workflow ID: wflow_NCUEIInw2VH4zaZE


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@angrybayblade angrybayblade merged commit b81b701 into master Dec 7, 2024
10 of 11 checks passed
@angrybayblade angrybayblade deleted the fix/enum-string-filter branch December 7, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants